refactor(world): encapsulate functionality [N-01] #2157

yonadaa · 2024-01-18T14:10:24Z

A few suggestions were fixed in previous PR's.

#2007 fixed registerTable:

The registerTable function might register a namespace as well.

#2007 fixed registerSystem:

The registerSystem function might register a namespace

#2096 added unregisterDelegation:

Individual delegations must be removed by overwriting them using the regular registration function, which still leaves a stray record in the UserDelegationControl table.

changeset-bot · 2024-01-18T14:10:29Z

⚠️ No Changeset found

Latest commit: b673d76

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/world/test/World.t.sol

packages/world/src/modules/core/implementations/AccessManagementSystem.sol

holic · 2024-01-18T16:28:57Z

packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol

+   * @dev Unregisters the system at the given ID
+   * @param systemId The unique identifier for the system
+   */
+  function unregisterSystem(ResourceId systemId) public virtual {


I think we intentionally don't allow for unregistering systems? cc @alvrs

from @alvrs via discord

yeah it's this weird situation where other contracts that interact with yours might break if you unregister function selectors or unregister systems, so I think we should only encourage upgrading systems in a non-breaking way
if someone really wants to remove a system or function selector they can do it by editing the tables, but i don't think it should be something we encourage

…functionality

holic · 2024-01-19T13:56:11Z

packages/world/test/World.t.sol

@@ -250,7 +251,9 @@ contract WorldTest is Test, GasReporter {
      coreRegistrationSystem.registerFunctionSelector.selector,
      coreRegistrationSystem.registerRootFunctionSelector.selector,
      coreRegistrationSystem.registerDelegation.selector,
-      coreRegistrationSystem.registerNamespaceDelegation.selector
+      coreRegistrationSystem.unregisterDelegation.selector,


oof, we didn't have this before, eh?

another reason for #2155

Yup, this was forgotten. Although this is a test file so whatever automation we add needs to apply here too.

alvrs · 2024-01-19T13:57:42Z

packages/world/src/modules/core/implementations/AccessManagementSystem.sol

+  function renounceOwnership(ResourceId namespaceId) public virtual onlyDelegatecall {
+    // Require the namespace ID to be a valid namespace
+    requireNamespace(namespaceId);
+
+    // Require the caller to own the namespace
+    AccessControl.requireOwner(namespaceId, _msgSender());
+
+    // Delete namespace owner
+    NamespaceOwner._deleteRecord(namespaceId);
+
+    // Revoke access from old owner
+    ResourceAccess._deleteRecord(namespaceId, _msgSender());
+  }


should we just call transferOwnership with address(0) here? Similar to the Ownable lib: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/e5c63635e3508a8d9d0afed091578cc4bb59a9c7/contracts/access/Ownable.sol#L77

Could transfer to address 0 and then delete the resource access record. No strong opinion though.

that would basically be the same logic but a few extra writes, wouldn't it?

This is an interesting pattern, but it ~doubles the gas from 35488 to 74700. Plus, setting then deleting the ResourceAccess record seems potentially more confusing then not? Unless folks feel strongly i think the inlined code is fine

I think this is fine how it is, but I did make a code suggestion below for completeness with other audit fixes (adding namespace existence check for better reverts)

packages/world/src/modules/core/implementations/AccessManagementSystem.sol

Co-authored-by: Kevin Ingersoll <[email protected]>

holic · 2024-01-19T14:55:06Z

packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol

+    // Require namespace ID to be a valid namespace
+    requireNamespace(namespaceId);
+
+    // Require the caller to own the namespace
+    AccessControl.requireOwner(namespaceId, _msgSender());


Suggested change

// Require namespace ID to be a valid namespace

requireNamespace(namespaceId);

// Require the caller to own the namespace

AccessControl.requireOwner(namespaceId, _msgSender());

// Require namespace ID to be a valid namespace

requireNamespace(namespaceId);

// Require the namespace to exist

AccessControl.requireExistence(namespaceId);

// Require the caller to own the namespace

AccessControl.requireOwner(namespaceId, _msgSender());

yonadaa requested review from alvrs and holic as code owners January 18, 2024 14:10

yonadaa marked this pull request as draft January 18, 2024 14:10

feat(world): add renounceOwnership function

15d6026

yonadaa force-pushed the yonadaaa/encapsulate-functionality branch from 7090e77 to 15d6026 Compare January 18, 2024 14:18

vercel bot deployed to Preview January 18, 2024 14:20 View deployment

feat: unregister namespace delegations

dcaff7b

vercel bot deployed to Preview January 18, 2024 14:35 View deployment

vercel bot deployed to Preview January 18, 2024 14:51 View deployment

feat: add unregisterSystem

06f2ca2

yonadaa force-pushed the yonadaaa/encapsulate-functionality branch from 5881958 to 06f2ca2 Compare January 18, 2024 14:53

vercel bot deployed to Preview January 18, 2024 14:55 View deployment

yonadaa marked this pull request as ready for review January 18, 2024 14:56

yonadaa commented Jan 18, 2024

View reviewed changes

packages/world/test/World.t.sol Show resolved Hide resolved

holic reviewed Jan 18, 2024

View reviewed changes

holic added 3 commits January 18, 2024 17:38

Merge remote-tracking branch 'origin/main' into yonadaaa/encapsulate-…

d88c6a7

…functionality

add namespace check

73b5ded

use new namespace check in unregisterNamespaceDelegation

4659afa

vercel bot deployed to Preview January 18, 2024 17:43 View deployment

holic marked this pull request as draft January 18, 2024 17:45

yonadaa added 2 commits January 19, 2024 13:24

Merge remote-tracking branch 'origin/main' into yonadaaa/encapsulate-…

0d23f3b

…functionality

chore: remove unregisterSystem

c7e270b

vercel bot deployed to Preview January 19, 2024 13:29 View deployment

fix: onlyDelegatecall

eaea087

yonadaa force-pushed the yonadaaa/encapsulate-functionality branch from 683cdb3 to eaea087 Compare January 19, 2024 13:33

vercel bot deployed to Preview January 19, 2024 13:35 View deployment

test: add unregisterNamespaceDelegation test

201ca27

yonadaa marked this pull request as ready for review January 19, 2024 13:45

comment

2cbc4fd

yonadaa requested a review from holic January 19, 2024 13:46

vercel bot deployed to Preview January 19, 2024 13:48 View deployment

holic reviewed Jan 19, 2024

View reviewed changes

alvrs reviewed Jan 19, 2024

View reviewed changes

holic reviewed Jan 19, 2024

View reviewed changes

packages/world/src/modules/core/implementations/AccessManagementSystem.sol Outdated Show resolved Hide resolved

test: add gas report for renounce

d45070d

vercel bot deployed to Preview January 19, 2024 14:51 View deployment

yonadaa and others added 2 commits January 19, 2024 14:53

fix: require existance on renounce

1860bbc

Co-authored-by: Kevin Ingersoll <[email protected]>

chore: gas report

a2c92af

holic reviewed Jan 19, 2024

View reviewed changes

fix: check existence in namespace

b673d76

yonadaa requested a review from holic January 19, 2024 14:58

vercel bot deployed to Preview January 19, 2024 14:59 View deployment

holic approved these changes Jan 19, 2024

View reviewed changes

yonadaa merged commit 61985bb into main Jan 19, 2024
11 checks passed

yonadaa deleted the yonadaaa/encapsulate-functionality branch January 19, 2024 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(world): encapsulate functionality [N-01] #2157

refactor(world): encapsulate functionality [N-01] #2157

yonadaa commented Jan 18, 2024 •

edited

Loading

changeset-bot bot commented Jan 18, 2024 •

edited

Loading

holic Jan 18, 2024

holic Jan 18, 2024

holic Jan 19, 2024

holic Jan 19, 2024

yonadaa Jan 19, 2024

alvrs Jan 19, 2024

alvrs Jan 19, 2024

holic Jan 19, 2024

yonadaa Jan 19, 2024 •

edited

Loading

holic Jan 19, 2024

holic Jan 19, 2024

refactor(world): encapsulate functionality [N-01] #2157

refactor(world): encapsulate functionality [N-01] #2157

Conversation

yonadaa commented Jan 18, 2024 • edited Loading

changeset-bot bot commented Jan 18, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonadaa Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonadaa commented Jan 18, 2024 •

edited

Loading

changeset-bot bot commented Jan 18, 2024 •

edited

Loading

yonadaa Jan 19, 2024 •

edited

Loading